Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated NSDate+Datetools category to reuse formatter instances. #13

Merged
merged 1 commit into from Apr 25, 2014
Merged

Conversation

cemaleker
Copy link

No description provided.

MatthewYork pushed a commit that referenced this pull request Apr 25, 2014
Updated NSDate+Datetools category to reuse formatter instances.
@MatthewYork MatthewYork merged commit 4de0540 into MatthewYork:master Apr 25, 2014
@@ -1514,9 +1501,14 @@ -(NSString *)formattedDateWithFormat:(NSString *)format locale:(NSLocale *)local
* @return NSString representing the formatted date string
*/
-(NSString *)formattedDateWithFormat:(NSString *)format timeZone:(NSTimeZone *)timeZone locale:(NSLocale *)locale{
NSDateFormatter *formatter = [[NSDateFormatter alloc] init];
static NSDateFormatter *formatter = nil;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry guys for saying this a little late, but this change makes this method thread-unsafe. Because NSDateFormatter is not thread safe, but NSDate is thread safe. Calling this method from different threads at the same time might cause an issue, because from the both threads the same NSDateFormatter object will be accessed.

Personally I use https://github.com/mysterioustrousers/MTDates for date calculations and this problem is solved there using locks. You could take a look at the implementation there.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. This, I believe, also extends to NSCalendar, which is used pervasively in the library. Feel free to raise an issue. Pull requests will be welcomed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatthewYork I'm sorry, I don't use this library now. I've noticed the library probably in cocoapods feed and started watching :) Unfortunately don't have enough of time to contribute to libraries I don't use, yet. I'm sorry.

Maybe I'll give this library a try on a next project in future ;)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yas375 I completely understand. I will create an issue and mark thread safety as an enhancement.

Cheers.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MatthewYork thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants